Fix SuppressMessage CustomRule #2142
Conversation
Problem: SuppressMessageAttribute failed when using named arguments for
RuleSuppressionID. Users could not use syntax like:
[SuppressMessage("RuleName", RuleSuppressionId="MyId")]
Root Cause: In RuleSuppression.cs, the named argument parser had two bugs:
1. Checked if RuleName was set instead of RuleSuppressionID
2. Assigned the value to RuleName instead of RuleSuppressionID
This broke selective rule suppression for custom rules.
Solution:
- Fixed conflict check to validate RuleSuppressionID instead of RuleName
- Fixed assignment to set RuleSuppressionID instead of RuleName
- Added comprehensive tests for named argument syntax
- Minor formatting improvements
Now both syntaxes work correctly:
[SuppressMessage("Rule", RuleSuppressionId="Id", Scope="Function")]
[SuppressMessage("Rule", "Id", Scope="Function")]
* Implemented a test case to validate the functionality of custom rules with targeted suppression. * The test recreates the scenario from GitHub issue PowerShell#1686, ensuring that `RuleSuppressionID` works correctly with named arguments. * Verified that violations are suppressed as expected based on the defined rules.
HeyItsGilbert
requested review from
andyleejordan and
bergmeister
as code owners
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the RuleSuppression attribute parser that prevented the use of named argument syntax for RuleSuppressionId. The bug caused [SuppressMessage("Rule", RuleSuppressionId="Id")] to fail, particularly affecting custom rules that use targeted suppression via RuleSuppressionID.
Key Changes:
- Fixed the named argument parser to check and assign
RuleSuppressionIDinstead of incorrectly checking/assigningRuleName - Added comprehensive test coverage for named argument syntax, mixed positional/named arguments, and custom rule scenarios
- Minor formatting improvements to test file (consistent whitespace in script blocks and preprocessor directives)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Engine/Generic/RuleSuppression.cs | Fixed two bugs in the named argument parser for RuleSuppressionID (lines 196, 201) and minor preprocessor directive formatting |
| Tests/Engine/RuleSuppression.tests.ps1 | Added 3 new comprehensive tests for named argument syntax including custom rule scenario, plus consistent whitespace formatting improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Thanks for taking the time for such a detailed fix and also adding tests to it. Basic code like this is unfortunately from initial days where the tool was just quickly coded up.
Happy as well @andyleejordan ?
andyleejordan
left a comment
andyleejordan
left a comment
There was a problem hiding this comment.
Good fix! My guess is, if it ever worked in the first place, a refactor is probably what broke it given it was the wrong argument being checked. But who knows.
3 participants
PR Summary
Problem: SuppressMessageAttribute failed when using named arguments for RuleSuppressionID. Users could not use syntax like: [SuppressMessage("RuleName", RuleSuppressionId="MyId")]
Root Cause: In RuleSuppression.cs, the named argument parser had two bugs:
This broke selective rule suppression for custom rules.
Solution:
Now both syntaxes work correctly:
[SuppressMessage("Rule", RuleSuppressionId="Id", Scope="Function")]
[SuppressMessage("Rule", "Id", Scope="Function")]
Fixes #1686
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.